substrate 2.0.0-rc4 version upgrade#1072
substrate 2.0.0-rc4 version upgrade#1072shamil-gadelshin merged 12 commits intoJoystream:content_directory_second_tryfrom iorveth:cont_dir_substrate_2_0
Conversation
iorveth
changed the title
iorveth
changed the title
|
I think I will just leave this review to @shamil-gadelshin |
shamil-gadelshin
left a comment
shamil-gadelshin
left a comment
There was a problem hiding this comment.
We don't have a strict rule for that but I recommend to comment all public methods or type definitions (for example in the storage definition): it could be done with temporary enabling this code line: #![warn(missing_docs)] .
Also, I like that for such a complex module it's very clean and understandable. Good work!
| frame-support = { package = 'frame-support', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'} | ||
| system = { package = 'frame-system', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'} | ||
| sp-arithmetic = { package = 'sp-arithmetic', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'} | ||
|
|
There was a problem hiding this comment.
Let's have a common format for dependencies: single line based similar to parity's format.
| sp-io = { package = 'sp-io', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'} | ||
| sp-core = { package = 'sp-core', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'} | ||
|
|
||
| [features] |
There was a problem hiding this comment.
Could you sort feature-definitions similar to dependencies order: it's easier to check them.
| std = [ | ||
| 'serde', | ||
| 'codec/std', | ||
| 'rstd/std', |
There was a problem hiding this comment.
Could you rename rstd to sp-std - similar to the new format in the Substrate?
| @@ -1,40 +1,34 @@ | |||
| [package] | |||
| name = 'substrate-content-directory-module' | |||
There was a problem hiding this comment.
Please rename it to 'pallet-content-directory'.
|
|
||
| /// Type, used in diffrent numeric constraints representations | ||
| type MaxNumber = u32; | ||
|
|
There was a problem hiding this comment.
If it's not too far out your current scope could you perform actual error conversion to the Substrate decl_error! macro?
|
We don't have a strict rule for that but I recommend to comment all public methods or type definitions (for example in the storage definition): it could be done with temporary enabling this code line: #![warn(missing_docs)] . Thanks for pointing that feature. Added missing comments for the all public functions, traits and structure fields. |
iorveth
changed the base branch from
content_directory_second_try
to
council_rework
iorveth
changed the base branch from
council_rework
to
content_directory_second_try
shamil-gadelshin
left a comment
shamil-gadelshin
left a comment
There was a problem hiding this comment.
LGTM. Nice work on error conversion!
3 participants
This PR aims to upgrade content directory to substrate version 2.0.0-rc4
Closes: #1054
To minimize merge conflict potential, this PR assumes #1064 will be merged first.